Skip to content

Conversation

@lbernstone
Copy link
Contributor

Description of Change

SD_MMC driver had been updated to support UHS-I on ESP-P4, but didn't have the flags quite right. This updates the driver to use UHS SDR by default.

Test Scenarios

ESP32-P4-EYE reading a file off SD card. Speed is improved about 300%.

Related links

Should close #12027

@lbernstone lbernstone requested a review from a team as a code owner November 17, 2025 19:47
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

Messages
📖 🎉 Good Job! All checks are passing!

👋 Hello lbernstone, we appreciate your contribution to this project!


📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against b333d4c

@lucasssvaz lucasssvaz requested a review from me-no-dev November 17, 2025 19:50
@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

MemoryFLASH [bytes]FLASH [%]RAM [bytes]RAM [%]
TargetDECINCDECINCDECINCDECINC
ESP32P40⚠️ +40.000.00000.000.00
ESP32S3000.000.00000.000.00
ESP32000.000.00000.000.00
Click to expand the detailed deltas report [usage change in BYTES]
TargetESP32P4ESP32S3ESP32
ExampleFLASHRAMFLASHRAMFLASHRAM
libraries/ESP_I2S/examples/Record_to_WAV⚠️ +400000
libraries/SD_MMC/examples/SD2USBMSC⚠️ +4000--
libraries/SD_MMC/examples/SDMMC_Test⚠️ +400000
libraries/SD_MMC/examples/SDMMC_time⚠️ +400000

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2025

Test Results

 76 files   76 suites   15m 47s ⏱️
 38 tests  38 ✅ 0 💤 0 ❌
241 runs  241 ✅ 0 💤 0 ❌

Results for commit b333d4c.

♻️ This comment has been updated with latest results.

@lucasssvaz
Copy link
Member

Please add a check for IDF versions greater than 5.3:

https://github.com/espressif/arduino-esp32/actions/runs/19442524813/job/55629051409?pr=12038

@P-R-O-C-H-Y P-R-O-C-H-Y self-requested a review November 18, 2025 07:41
@P-R-O-C-H-Y
Copy link
Member

On P4 EV Board card is no longer mounting with the changes in PR.

@P-R-O-C-H-Y
Copy link
Member

@lbernstone If I comment out the UHS-1 flag, the sdmmc starts working again and the speed is improved with the DDR flag only.

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: In Progress ⚠️ Issue is in progress Area: Libraries Issue is related to Library support. labels Nov 18, 2025
@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

@P-R-O-C-H-Y What kind of SD card do you have?
Does it have UHS markings on the card or is it just a SDHC/SDXC card? (or even just SD, which is really a collectors item these days)

@P-R-O-C-H-Y
Copy link
Member

@P-R-O-C-H-Y What kind of SD card do you have? Does it have UHS markings on the card or is it just a SDHC/SDXC card? (or even just SD, which is really a collectors item these days)

Its a Kingston 16GB with USH-1 marking on it.

@P-R-O-C-H-Y
Copy link
Member

Weird is, that the SD card is not working on the P4-EYE board I got. Even with/without the changes in this PR.

@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

Can you try to format it using some SD-format tool?
Or format it using some non-PC device like a camera.
It might be the partitioning is unconventional and/or the file system already present is not supported by the libraries we use.
Or maybe the "P4 EYE" doesn't use all 4 pins? For example DAT3 is wired as card detection?

@P-R-O-C-H-Y
Copy link
Member

The P4-eye have identical SD card schema as the P4-EV board. Will try with formatted card and also with different one.

@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

Maybe also try some SDHC/SDXC card?
Those UHS cards may want to negotiate a lower signal level of 1V8. Not sure if this is expected or required. I know it is a feature of the standard.

@me-no-dev
Copy link
Member

in all cases, we want to support more cards, not less. If these options make cards not work, we need to make them optional

@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

I'm not sure if SDHC uses DDR signalling.
If I'm not mistaken, this was initially only a protocol update to support cards > 2 GB.
There is a "high speed" variant for SDHC/XC/UC which doubles the bus speed, so I guess that does look like DDR.

Looking here: https://www.sdcard.org/developers/sd-standard-overview/bus-speed-default-speed-high-speed-uhs-sd-express/
I see there is a 50 MHz and 104 MHz signalling for UHS-I, where there doesn't seem to be a DDR104.
I have no idea what frequency is being used here?

@lbernstone
Copy link
Contributor Author

I'll see if I can find some old cards around here and test an array of them. Not sure if I have really small & slow ones around any more.

@lbernstone
Copy link
Contributor Author

lbernstone commented Nov 18, 2025

Weird is, that the SD card is not working on the P4-EYE board I got. Even with/without the changes in this PR.

P4-eye (waveshare) has BOARD_SDMMC_POWER_PIN on 46

@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

Hmm I took another look at the source code of this PR and

host.flags &= ~SDMMC_HOST_FLAG_DDR;

looks like it is disabling DDR?
If so then I am quite curious to why it is improving the speed.

If it is indeed increasing the transfer speed, then it sounds like a timing issue, or maybe some signal is inverted?

@TD-er
Copy link
Contributor

TD-er commented Nov 18, 2025

Hmm these defines for sure would be a lot more clear with quite a bit more comments...
https://github.com/espressif/esp-idf/blob/8f1e7bc4e08878f5a2aab55fdf302789d3a3eac4/components/sdmmc/include/sd_protocol_types.h#L198-L205

#define SDMMC_FREQ_DEFAULT      20000       /*!< SD/MMC Default speed (limited by clock divider) */
#define SDMMC_FREQ_HIGHSPEED    40000       /*!< SD High speed (limited by clock divider) */
#define SDMMC_FREQ_PROBING      400         /*!< SD/MMC probing speed */
#define SDMMC_FREQ_52M          52000       /*!< MMC 52MHz speed */
#define SDMMC_FREQ_26M          26000       /*!< MMC 26MHz speed */
#define SDMMC_FREQ_DDR50        50000       /*!< MMC 50MHz speed */
#define SDMMC_FREQ_SDR50        100000      /*!< MMC 100MHz speed */
#define SDMMC_FREQ_SDR104       200000      /*!< MMC 200MHz speed */

The names of those defines don't appear to match the frequency numbers.
I assume the "DDR50", "SDR50" and "SDR104" refer to the bus speeds on this page
If so, then both DDR50 and SDR50 refer to 50 MByte/sec.
And since there are 4 bits per transfer on UHS-I cards (not sure about the others as they do have 2 rows of pads, always thought those were for differential signalling, but not 100% sure), you need 2 transfers per byte.
So for SDR50 -> 100 MHz and DDR50 -> 50MHz

Then the most important change of this PR is setting the clock?
And then not disabling DDR and setting the max. freq to SDMMC_FREQ_DDR50 would result in the same speed improvement, right?
Likely with less RF noise and better timings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Libraries Issue is related to Library support. Status: In Progress ⚠️ Issue is in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SD_MMC is unexpectedly slow in ESP32-P4

5 participants